KAFKA-6726: Fine Grained ACL for CreateTopics (KIP-277) by edoardocomar · Pull Request #4795 · apache/kafka

edoardocomar · 2018-03-29T14:02:20Z

Handling CreateTopicsRequest now requires Create auth on Topic resource and does not require Create on Cluster
AclCommand --producer option adjusted
Existing Unit and Integration tests adjusted accordingly

https://issues.apache.org/jira/browse/KAFKA-6726
https://cwiki.apache.org/confluence/display/KAFKA/KIP-277+-+Fine+Grained+ACL+for+CreateTopics+API

Co-authored-by: Edoardo Comar ecomar@uk.ibm.com
Co-authored-by: Mickael Maison mickael.maison@gmail.com

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

ijuma · 2018-05-24T21:36:11Z

Thanks for the PR, let's add an upgrade note, please.

edoardocomar · 2018-05-25T14:44:14Z

Hi @ijuma thanks for reviewing
we rebased, marginally improved testing for auto-create and added upgrade notes done

edoardocomar · 2018-05-29T13:16:51Z

@ijuma last commit was just to resolve merge conflicts

edoardocomar · 2018-05-29T16:21:41Z

test failure unrelated (dynamic broker config)

@ijuma can you please review as the deadline for feature freeze is tomorrow

ijuma · 2018-05-29T18:05:14Z

This is a small feature, so there's an extra week.

ijuma

Thanks for the PR, a few minor comments after the first pass.

ijuma · 2018-06-01T07:22:54Z

core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala

It's safer to have asserts for both cases here and in the other place.

Ok, added test cases for all code paths

ijuma · 2018-06-01T07:24:55Z

core/src/test/scala/integration/kafka/api/EndToEndAuthorizationTest.scala

Do we need new tests here given AuthorizerIntegrationTest.

yes, we think so because EndToEndAuthorizationTest uses the acl tool command line

Don't we have AclCommandTest for that?

Generally, this test was meant to cover some high level end to end cases. It seems OK to add an auto create case, but overkill to add one for Create Topic and another for Create Cluster. We should add a test to AclCommandTest for the latter.

Thanks, I see the AclCommandTest has already coverage of adding cluster create and topic create acls. The AuthorizerIntegrationTest covers all request handling logic with and without auto-create.

So we only left one end2end test case for auto create with topic create acl

ijuma · 2018-06-01T07:30:47Z

core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala

It's a bit odd that we are using maps here even though there's a single entry in each. Shall we simplify?

* Handling CreateTopicsRequest now requires Create auth on Topic resource and does not require Create on Cluster * AclCommand --producer option adjusted * Existing Unit and Integration tests adjusted accordingly https://issues.apache.org/jira/browse/KAFKA-6726 https://cwiki.apache.org/confluence/display/KAFKA/KIP-277+-+Fine+Grained+ACL+for+CreateTopics+API Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com> Co-authored-by: Mickael Maison <mickael.maison@gmail.com> - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes)

* Handling CreateTopicsRequest now requires Create auth on Topic resource and does not require Create on Cluster * AclCommand --producer option adjusted * Existing Unit and Integration tests adjusted accordingly * upgrade notes https://issues.apache.org/jira/browse/KAFKA-6726 https://cwiki.apache.org/confluence/display/KAFKA/KIP-277+-+Fine+Grained+ACL+for+CreateTopics+API Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com> Co-authored-by: Mickael Maison <mickael.maison@gmail.com> - [ ] Verify design and implementation - [ ] Verify test coverage and CI build status - [ ] Verify documentation (including upgrade notes)

check all paths on topic auto creation simplified testCreateTopicAuthorizationWithClusterCreate

edoardocomar · 2018-06-04T17:27:48Z

Hi @ijuma thanks for the review - we rebased to address the conflicts and also addressed at your comments

you will notice a weird double call to consumers.head.poll(Duration.ofMillis(50L));
as we wanted the test to check topic auto-creation without a write request but for some reason (unrelated to our changes) the first call to poll after a previously failed one did not trigger the topic auto-creation.

edoardocomar · 2018-06-05T09:50:17Z

about the need for a different poll timeout - I opened another jira, does not seem a critical bug
https://issues.apache.org/jira/browse/KAFKA-6994

ijuma

A couple of quick questions while I review the rest.

ijuma · 2018-06-05T10:17:26Z

core/src/main/scala/kafka/admin/AclCommand.scala

-      (Resource.ClusterResource -> (getAcl(opts, Set(Create)) ++
-        (if (enableIdempotence) getAcl(opts, Set(IdempotentWrite)) else Set.empty[Acl])))
+    //Write, Describe, Create permission on topics, Write, Describe on transactionalIds
+    topics.map(_ -> topicAcls).toMap[Resource, Set[Acl]] ++


Can toMap[Resource, Set[Acl]] be simply toMap? Same for other cases.

thanks, existing syntax simplified

ijuma · 2018-06-05T10:19:12Z

core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala

-      this.consumers.head.assign(List(topicPartition).asJava)
-      consumeRecords(this.consumers.head)
-      Assert.fail("should have thrown exception")
+      this.consumers.head.poll(Duration.ofMillis(50L));


Why did we replace consumeRecords with poll?

We modified the method to avoid producing any records - to avoid adding write permission which made the test more complicated than it needed to be.

So we need to pass numRecords=0 and we had no control on the timeout, unless we made that another optional argument.
without actually consuming records it seems to me that just calling poll is simpler than consumeRecords

We want to be generous with the timeout then. Any reason why it's so low?

the default timeout was fine for the first call (when we expect it always to throw)

it's the second one that caused issues - see https://issues.apache.org/jira/browse/KAFKA-6994 - but that IMHO is unrelated to the changes in this PR.

if the second timeout is low - even without adding the required acl the test passed - that's the bug 6994.

if the second timeout is large the test takes longer, that's why we kept relatively short

Did you see my comment in KAFKA-6994?

yep - replied there - it's the same with the old poll

Even then, poll provides little in the way of guarantees. I suggest using TestUtils.retry with a generous timeout and a short timeout for poll. This also ensures we finish as quickly as possible.

good point - we can use waitUntilTrue checking for the topic being created as a consequence of poll

ijuma · 2018-06-05T10:50:30Z

core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala

+
+    sendRequestAndVerifyResponseError(ApiKeys.CREATE_TOPICS, createTopicsRequest, resources, isAuthorized = false)
+
+    clusterCreateAcl.get(topicResource).foreach { acls =>


This is a bit dangerous because if get returns None, the test will pass.

Thanks! - the test did not actually need that block, (get was returning None !) was a sloppy remainder of previous refactoring

ijuma · 2018-06-05T11:23:57Z

core/src/main/scala/kafka/server/KafkaApis.scala

          } else Map.empty

-        val completeResults = results ++ duplicatedTopicsResults
+        val unauthorizedTopicsResults =  unauthorizedTopics.keySet.map(_ -> new ApiError(Errors.TOPIC_AUTHORIZATION_FAILED, null)) 


Extra space after =.

ijuma · 2018-06-05T11:28:22Z

core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala

-    val newTopic = "newTopic"
+  def testCreatePermissionOnTopicToWriteToNonExistentTopic() {
+    testCreatePermissionNeededToWriteToNonExistentTopic("newTopic",
+      Set(new Acl(userPrincipal, Allow, Acl.WildCardHost, Write), new Acl(userPrincipal, Allow, Acl.WildCardHost, Create)),


Why do we need Write here?

thanks, going to simplify by removing the constant arguments

ijuma · 2018-06-05T11:44:14Z

core/src/test/scala/integration/kafka/api/AuthorizerIntegrationTest.scala

-      this.consumers.head.assign(List(topicPartition).asJava)
-      consumeRecords(this.consumers.head)
-      Assert.fail("should have thrown exception")
+      this.consumers.head.poll(Duration.ofMillis(50L));


Even then, poll provides little in the way of guarantees. I suggest using TestUtils.retry with a generous timeout and a short timeout for poll. This also ensures we finish as quickly as possible.

ijuma · 2018-06-05T11:45:00Z

core/src/test/scala/integration/kafka/api/EndToEndAuthorizationTest.scala

  this.serverConfig.setProperty(KafkaConfig.OffsetsTopicPartitionsProp, "1")
  this.serverConfig.setProperty(KafkaConfig.OffsetsTopicReplicationFactorProp, "3")
  this.serverConfig.setProperty(KafkaConfig.MinInSyncReplicasProp, "3")
+  this.serverConfig.setProperty(KafkaConfig.DefaultReplicationFactorProp, "3")


Why is this needed?

because we added tests which use topic auto-create, so they need a default replication factor matching the min in sync

thanks anyway, we spotted a missing @Test :(

ijuma · 2018-06-05T11:48:43Z

core/src/main/scala/kafka/server/KafkaApis.scala

        if (!authorize(request.session, Create, Resource.ClusterResource)) {
-          authorizedTopics --= nonExistingTopics
-          unauthorizedForCreateTopics ++= nonExistingTopics
+          val unauthorizedForCreateTopics = authorizedTopics.filter(topic => !authorize(request.session, Create, new Resource(Topic, topic)))


Nit: long line.

ijuma · 2018-06-05T11:53:20Z

core/src/main/scala/kafka/server/KafkaApis.scala

      topics.partition(topic => authorize(request.session, Describe, new Resource(Topic, topic)))

-    var unauthorizedForCreateTopics = Set[String]()
+    var authorizedForDescribeNotCreateTopics = Set[String]()


Why did we rename this?

the new name though longer and not pretty matches the what the set of topics actually is in terms of authorizations

But then later we still have things like unauthorizedForCreateTopicMetadata. I think we should just keep this variable as it was since there is no actual change here. I'll push a minor commit.

ijuma · 2018-06-06T06:24:24Z

core/src/main/scala/kafka/server/KafkaApis.scala

        if (!authorize(request.session, Create, Resource.ClusterResource)) {
-          authorizedTopics --= nonExistingTopics
-          unauthorizedForCreateTopics ++= nonExistingTopics
+          val unauthorizedForCreateTopics = authorizedTopics.filter(


This seems like a bug, we should only be doing this check for nonExistingTopics. Will push a fix.

…grained-acl-create-topics * apache-github/trunk: KAFKA-5588: Remove deprecated --new-consumer tools option (apache#5097) MINOR: Fix for the location of the trogdor.sh executable file in the documentation. (apache#5040) KAFKA-6997: Exclude test-sources.jar when $INCLUDE_TEST_JARS is FALSE MINOR: docs should point to latest version (apache#5132) KAFKA-6981: Move the error handling configuration properties into the ConnectorConfig and SinkConnectorConfig classes (KIP-298) [KAFKA-6730] Simplify State Store Recovery (apache#5013) MINOR: Rename package `internal` to `internals` for consistency (apache#5137) KAFKA-6704: InvalidStateStoreException from IQ when StreamThread closes store (apache#4801) MINOR: Add missing configs for resilience settings MINOR: Add regression tests for KTable mapValues and filter (apache#5134) KAFKA-6750: Add listener name to authentication context (KIP-282) (apache#4829) KAFKA-3665: Enable TLS hostname verification by default (KIP-294) (apache#4956) KAFKA-6938: Add documentation for accessing Headers on Kafka Streams Processor API (apache#5128) KAFKA-6813: return to double-counting for count topology names (apache#5075) KAFKA-5919; Adding checks on "version" field for tools using it MINOR: Remove deprecated KafkaStreams constructors in docs (apache#5118)

ijuma

I pushed a few minor changes including a merge conflict fix. This PR LGTM now and I will merge once the tests pass. @edoardocomar, please follow up if you have concerns about any of my changes.

omkreddy · 2018-06-06T09:00:41Z

LGTM

…refix * apache-github/trunk: KAFKA-6726: Fine Grained ACL for CreateTopics (KIP-277) (apache#4795) KAFKA-5588: Remove deprecated --new-consumer tools option (apache#5097) MINOR: Fix for the location of the trogdor.sh executable file in the documentation. (apache#5040) KAFKA-6997: Exclude test-sources.jar when $INCLUDE_TEST_JARS is FALSE MINOR: docs should point to latest version (apache#5132) KAFKA-6981: Move the error handling configuration properties into the ConnectorConfig and SinkConnectorConfig classes (KIP-298) [KAFKA-6730] Simplify State Store Recovery (apache#5013) MINOR: Rename package `internal` to `internals` for consistency (apache#5137) KAFKA-6704: InvalidStateStoreException from IQ when StreamThread closes store (apache#4801) MINOR: Add missing configs for resilience settings MINOR: Add regression tests for KTable mapValues and filter (apache#5134) KAFKA-6750: Add listener name to authentication context (KIP-282) (apache#4829) KAFKA-3665: Enable TLS hostname verification by default (KIP-294) (apache#4956) KAFKA-6938: Add documentation for accessing Headers on Kafka Streams Processor API (apache#5128) KAFKA-6813: return to double-counting for count topology names (apache#5075) KAFKA-5919; Adding checks on "version" field for tools using it MINOR: Remove deprecated KafkaStreams constructors in docs (apache#5118)

edoardocomar · 2018-06-06T15:17:55Z

thanks @ijuma

edoardocomar · 2018-06-06T16:17:06Z

@ijuma we looked closely at your commit dc2727e
great that you spotted that we split the wrong collection ...
we added a minor PR to cover that case: #5155

ijuma · 2018-06-06T16:55:49Z

@edoardocomar you read my mind as I was going to ask if you were willing to submit a PR to fix the testing gap. :)

- CreateTopicsRequest now requires Create auth on Topic resource or Create on Cluster resource. - AclCommand --producer option adjusted - Existing unit and Integration tests adjusted accordingly and new tests added. Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>, Ismael Juma <ismael@juma.me.uk> Co-authored-by: Edoardo Comar <ecomar@uk.ibm.com> Co-authored-by: Mickael Maison <mickael.maison@gmail.com>

mimaison force-pushed the topic_create_acl branch from 82b81b4 to 6af9eae Compare April 30, 2018 14:18

edoardocomar force-pushed the topic_create_acl branch from a1a014f to 6f02824 Compare May 1, 2018 11:19

ijuma self-requested a review May 21, 2018 18:58

ijuma added this to the 2.0.0 milestone May 21, 2018

ijuma self-assigned this May 21, 2018

edoardocomar force-pushed the topic_create_acl branch from 6f02824 to 7182df0 Compare May 25, 2018 13:02

edoardocomar force-pushed the topic_create_acl branch from 6f7ff10 to c6e954a Compare May 29, 2018 13:16

edoardocomar force-pushed the topic_create_acl branch 2 times, most recently from 03d6a33 to f9034eb Compare May 31, 2018 09:06

ijuma reviewed Jun 1, 2018

View reviewed changes

mimaison and others added 5 commits June 4, 2018 16:11

upgrade notes

1a1584c

minor improvement to test with auto-create option

099c539

update following review

ea58fc3

check all paths on topic auto creation simplified testCreateTopicAuthorizationWithClusterCreate

edoardocomar force-pushed the topic_create_acl branch from f9034eb to ea58fc3 Compare June 4, 2018 17:22

ijuma changed the title ~~KAFKA-6726: KIP-277 - Fine Grained ACL for CreateTopics API~~ KAFKA-6726: Fine Grained ACL for CreateTopics API (KIP-277) Jun 5, 2018

ijuma changed the title ~~KAFKA-6726: Fine Grained ACL for CreateTopics API (KIP-277)~~ KAFKA-6726: Fine Grained ACL for CreateTopics (KIP-277) Jun 5, 2018

avoiding double poll in test

afd55d7

ijuma reviewed Jun 5, 2018

View reviewed changes

edoardocomar added 2 commits June 5, 2018 11:50

syntax simplification following review

c3bf9fe

addressed review comment

b86c3c2

ijuma reviewed Jun 5, 2018

View reviewed changes

edoardocomar added 6 commits June 5, 2018 14:04

using TestUtil loop to check for topic existence

59d1822

fixed extra space

8226bfc

addressed review comment

d4e5fc4

missing @test annotation

d82f725

split long line

ce3db5a

removed overkill testProduceConsumeTopicAutoCreateClusterCreateAcl

b89ff7a

ijuma reviewed Jun 6, 2018

View reviewed changes

ijuma added 4 commits June 5, 2018 23:43

Fix bug in KafkaApis

dc2727e

Minor tweaks in AuthorizerIntegrationTest

c37272f

Remove dead code and some renames in EndToEndAuthorizationTest

0276cf0

ijuma approved these changes Jun 6, 2018

View reviewed changes

ijuma merged commit 0c035c4 into apache:trunk Jun 6, 2018

edoardocomar deleted the topic_create_acl branch June 6, 2018 18:39

edoardocomar mentioned this pull request Jun 13, 2018

MINOR: follow-up test for create topic acl (KIP-277) #5155

Merged

3 tasks


		sendRequestAndVerifyResponseError(ApiKeys.CREATE_TOPICS, createTopicsRequest, resources, isAuthorized = false)

		clusterCreateAcl.get(topicResource).foreach { acls =>

Comments

Conversation

edoardocomar commented Mar 29, 2018

Committer Checklist (excluded from commit message)

Uh oh!

ijuma commented May 24, 2018

Uh oh!

edoardocomar commented May 25, 2018

Uh oh!

edoardocomar commented May 29, 2018

Uh oh!

edoardocomar commented May 29, 2018

Uh oh!

ijuma commented May 29, 2018

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edoardocomar commented Jun 4, 2018

Uh oh!

edoardocomar commented Jun 5, 2018

Uh oh!

ijuma left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edoardocomar Jun 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

edoardocomar Jun 5, 2018 •

edited

Loading